Skip to content

Assert redundant incremental check#156599

Open
zetanumbers wants to merge 1 commit into
rust-lang:mainfrom
zetanumbers:assert_loadable_from_disk
Open

Assert redundant incremental check#156599
zetanumbers wants to merge 1 commit into
rust-lang:mainfrom
zetanumbers:assert_loadable_from_disk

Conversation

@zetanumbers
Copy link
Copy Markdown
Contributor

@zetanumbers zetanumbers commented May 15, 2026

So to incrementally execute tcx.ensure_done().query() we first do query graph coloring to determine if nothing has changed and maybe color our query green. In that case for ensure_ok we skip further execution and return. However for ensure_done we do this check:

rust/compiler/rustc_query_impl/src/execution.rs

Lines 601 to 608 in 7c3c88f

// In ensure-done mode, we can only skip execution for this key
// if there's a disk-cached value available to load later if
// needed, which guarantees the query provider will never run
// for this key.
EnsureMode::Done => {
(query.will_cache_on_disk_for_key_fn)(key)
&& loadable_from_disk(tcx, serialized_dep_node_index)
}

Which in turn looks up a serialized query value:

rust/compiler/rustc_middle/src/query/on_disk_cache.rs

Lines 338 to 343 in 7c3c88f

/// Returns true if there is a disk-cached query return value for the given node.
#[inline]
pub fn loadable_from_disk(&self, dep_node_index: SerializedDepNodeIndex) -> bool {
self.query_values_index.contains_key(&dep_node_index)
// with_decoder is infallible, so we can stop here
}

However, we only serialize query values if the pure (I've checked) function will_cache_on_disk_for_key_fn from above returns true:

rust/compiler/rustc_query_impl/src/plumbing.rs

Lines 95 to 99 in 7c3c88f

query.cache.for_each(&mut |key, value, dep_node| {
if (query.will_cache_on_disk_for_key_fn)(*key) {
encoder.encode_query_value::<V>(dep_node, &erase::restore_val::<V>(*value));
}
});

As such loadable_from_disk should be able to simply check if tcx.query_system.on_disk_cache is loaded and assume query value is present, assuming that will_cache_on_disk_for_key_fn returned true.

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 15, 2026

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, query-system
  • compiler, query-system expanded to 73 candidates
  • Random selection from 17 candidates

@zetanumbers
Copy link
Copy Markdown
Contributor Author

zetanumbers commented May 15, 2026

I think we can clarify this change with a comment or by outright inlining that function. Not sure what to pick tho.

@zetanumbers
Copy link
Copy Markdown
Contributor Author

zetanumbers commented May 15, 2026 •
edited
Loading

Also that check is covered by tests, so I making it unconditionally panic would trigger following incremental tests failures:

  • change_crate_dep_kind.rs
  • const-generics/change-const-param-type.rs
  • const-generics/change-const-param-gat.rs
  • hashes/consts.rs
  • define-opaques.rs
  • hashes/closure_expressions.rs
  • feature_gate.rs
  • commandline-args.rs
  • hashes/call_expressions.rs
  • hashes/enum_constructors.rs
  • hashes/if_expressions.rs
  • hashes/loop_expressions.rs
  • hashes/function_interfaces.rs
  • hashes/for_loops.rs
  • hashes/inherent_impls.rs
  • hashes/struct_constructors.rs
  • hashes/while_loops.rs
  • hashes/statics.rs
  • hashes/while_let_loops.rs
  • hashes/trait_impls.rs
  • ich_nested_items.rs
  • issue-100521-change-struct-name-assocty.rs
  • issue-49595/issue-49595.rs
  • reorder_vtable.rs
  • spans_significant_w_panic.rs
  • spans_significant_w_debuginfo.rs
  • issue-110457-same-span-closures/main.rs
  • static_cycle/b.rs
  • static_refering_to_other_static2/issue.rs
  • static_refering_to_other_static3/issue.rs
  • thinlto/cgu_keeps_identical_fn.rs
  • static_stable_hash/issue-49301.rs
  • thinlto/cgu_invalidated_when_export_added.rs
  • unrecoverable_query.rs

@zetanumbers
Copy link
Copy Markdown
Contributor Author

zetanumbers commented May 15, 2026

I think we could change it to debug_assert and check if perf is affected.

@chenyukang
Copy link
Copy Markdown
Member

chenyukang commented May 18, 2026

@rustbot reroll

@rustbot rustbot assigned TaKO8Ki and unassigned chenyukang May 18, 2026
@Zalathar
Copy link
Copy Markdown
Member

Zalathar commented May 19, 2026

I think we can clarify this change with a comment or by outright inlining that function. Not sure what to pick tho.

Making the existing helper function assert seems like a hazard if any other code starts calling it, so I think it's clearer to inline and remove the function.

After inlining I think we can also get rid of the else arm and call on_disk_cache.as_ref().unwrap(), because the disk-cache struct should always be present in incremental sessions, even if no previous session was loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@TaKO8Ki TaKO8Ki

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zetanumbers @rustbot @chenyukang @Zalathar @TaKO8Ki